[Reporting] Decouple screenshotting plugin from the reporting #120110

dokmic · 2021-12-01T14:47:41Z

Summary

Resolves #116503. This plugin moves all the screenshot taking logic out of the reporting plugin.

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

tsullivan · 2021-12-01T20:49:11Z

x-pack/plugins/reporting/server/core.ts

+
+          layout: {
+            zoom: config.get('capture', 'zoom'),
+            ...options.layout,


It feels like all of these config options could be moved over and given the renameFromRoot treatment, as the others have. Let's do that in another PR, unless you object.

xpack.reporting.capture.timeouts

xpack.reporting.capture.zoom

xpack.reporting.capture.loadDelay

x-pack/plugins/reporting/server/core.ts

x-pack/plugins/screenshotting/server/browsers/chromium/paths.ts

jloleysens

This is looking really good so far @dokmic !

Might be a pre-existing issue but generating a PDF report from Canvas resulted in:

[2021-12-03T15:24:37.939+01:00][WARN ][plugins.screenshotting.screenshot.browser-driver] Reporting encountered an uncaught error on the page that will be ignored: Error: TypeError: Cannot read property 'setScreenshotLayout' of undefined
    at <anonymous>:1:31
    at <anonymous>:1:58

being logged. This was really just a warning since the report did generate successfully. Not sure why that happened though, could be that Canvas just does not depend on screenshot mode.

I tested in Canvas and Dashboard and everything worked as expected. Great work!

[UPDATE]

Happy to merge once CI is green ;)

jloleysens · 2021-12-02T15:37:56Z

x-pack/plugins/reporting/public/management/components/report_info_flyout_content.tsx

-    {
-      title: i18n.translate('xpack.reporting.listing.infoPanel.browserTypeInfo', {
-        defaultMessage: 'Browser type',
-      }),
-      description: info.browser_type || NA,
-    },


Thanks for cleaning this up!

tsullivan · 2021-12-03T19:23:25Z

x-pack/plugins/reporting/server/export_types/png_v2/execute_job.ts

-          jobLogger.error(err);
-          return Rx.throwError(err);
-        }),
+        tap({ error: (error) => jobLogger.error(error) }),


much better 😅

tsullivan

LGTM

Pulled the branch and tested the code. Did yarn build to test in a VM.

Reviewed the code by merging main and looking at git diff -M -w main

elasticmachine · 2021-12-06T08:59:07Z

Pinging @elastic/kibana-app-services (Team:AppServicesUx)

dokmic · 2021-12-06T11:23:38Z

Might be a pre-existing issue but generating a PDF report from Canvas resulted in:

[2021-12-03T15:24:37.939+01:00][WARN ][plugins.screenshotting.screenshot.browser-driver] Reporting encountered an uncaught error on the page that will be ignored: Error: TypeError: Cannot read property 'setScreenshotLayout' of undefined
    at <anonymous>:1:31
    at <anonymous>:1:58

That was an actual bug that is fixed now. Thanks, @jloleysens.

jloleysens

Did not re-test, but approving based on previous testing. Great work!

kibana-ci · 2021-12-06T12:36:45Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`screenshotting`	-	8	+8

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`reporting`	162	163	+1
`screenshotting`	-	11	+11
total			+12

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`reporting`	56.3KB	56.3KB	-41.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`reporting`	14	15	+1
`screenshotting`	-	5	+5
total			+6

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`reporting`	42.1KB	41.8KB	-224.0B
`screenshotting`	-	2.0KB	+2.0KB
total			+1.8KB

Unknown metric groups

API count

id	before	after	diff
`reporting`	165	166	+1
`screenshotting`	-	25	+25
total			+26

References to deprecated APIs

id	before	after	diff
`reporting`	27	25	-2
`screenshotting`	-	12	+12
total			+10

History

💚 Build #11124 succeeded 7b72877ce019085f12fb0f3f76d0111a87a85923
💔 Build #11051 failed 6032c787d611dfbcc02b92ee51eea230a5aac78f
💔 Build #11014 failed 6eb03107480d9d63288fda037ad3e54c5a331a49
💔 Build #10976 failed e4561721d7745ae263eb673146579b8253c2c724
💔 Build #10957 failed 62255c80c57cb4e31e63934527750d1a42412af0
💔 Build #10945 failed 66391b48ba595f178ef140dc3a5611dc821e40f7

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

TinaHeiligers

Removing the browser_type usage field is ok for telemetry. Removing the field from the index mapping would require a reindex, as the indices are strictly mapped.
LGTM

spalger · 2021-12-06T17:37:24Z

packages/kbn-optimizer/limits.yml

@@ -117,3 +117,4 @@ pageLoadAssetSize:
  dataViewManagement: 5000
  reporting: 57003
  visTypeHeatmap: 25340
+  screenshotting: 17017


Do you mind helping me understand the value of screenshotting adding a new 2kb request to every page load? It looks like it stored a very tiny amount of state which could easily be managed by the reporting plugin and just increases the overall page load size and request count, but I'm probably missing something.

If there isn't a good reason it would be great if we could set "ui": false to the kibana.json file and move the contextStorage stuff to the reporting plugin.

That exposes a generic service working with the screenshotting context. The idea is that some of the plugins will use that to extract some serialized data without having a dependency on the reporting. This code will be merged with the screenshotMode plugin later, so won't stay long here.

dmlemeshko

LGTM

…-chromium-before-compiling-pdf * 'main' of github.com:elastic/kibana: (121 commits) FTR should use the new kibana_system user (elastic#120436) [Lens] Temporarily exclude Mosaic/Waffle from the suggestions list (elastic#120488) [Reporting] Fix slow CSV with large max size bytes (elastic#120365) [CTI] Threat Intel Card on Overview page needs to accommodate Fleet TI integrations - (elastic#120459) [Dashboard] Added KibanaThemeProvider. (elastic#120122) add more rule_registry unit tests (elastic#120323) [Lens] update xpack.lens.pie.smallValuesWarningMessage text (elastic#120478) [Fleet] Simplified package policy create API, enriching default values (elastic#119739) mock `elastic-apm-node` in `@kbn/test` jest preset (elastic#120324) [RAC] Rename occurrences of alert_type to rule_type in Infra (elastic#120455) [Security Solutions] Removes tech debt of exporting all from linter rule for timeline plugin (elastic#120437) [Workplace Search] Add API Keys view to replace Access tokens (elastic#120147) [Security Solutions] Removes tech debt of exporting all from linter rule for cases plugin in the server section (elastic#120411) Add support for threat.feed.name (elastic#120250) [Rule Registry] Rewrite APM registry rules for Observability (elastic#117740) [docs] Fix artifact layout formatting (elastic#119386) [Workplace Search] Add a technical preview of connecting GitHub via GitHub apps (elastic#119764) add osquery notes for 7.16 (elastic#120407) chore(NA): splits types from code on @kbn/docs-utils (elastic#120533) [Reporting] Decouple screenshotting plugin from the reporting (elastic#120110) ... # Conflicts: # x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.test.ts # x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts # x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts # x-pack/plugins/reporting/server/lib/screenshots/observable.ts # x-pack/plugins/reporting/server/test_helpers/create_mock_browserdriverfactory.ts

yakhinvadim · 2021-12-07T19:12:17Z

I'm on M1X chip and I see an error when running yarn start that looks related to this PR.

Here's what I have in the console:

[2021-12-07T11:02:37.881-08:00][INFO ][plugins.screenshotting.config] Chromium sandbox provides an additional layer of protection, and is supported for Darwin OS. Automatically enabling Chromium sandbox.
[2021-12-07T11:02:37.881-08:00][ERROR][plugins.screenshotting] Error in screenshotting setup, it may not function properly.
Unhandled Promise rejection detected:

Error: Unsupported platform: darwin-arm64
    at install (/Users/yakhinvadim/Projects/kibana/x-pack/plugins/screenshotting/server/browsers/install.ts:37:11)
    at /Users/yakhinvadim/Projects/kibana/x-pack/plugins/screenshotting/server/plugin.ts:47:132
    at ScreenshottingPlugin.setup (/Users/yakhinvadim/Projects/kibana/x-pack/plugins/screenshotting/server/plugin.ts:53:7)
    at PluginWrapper.setup (/Users/yakhinvadim/Projects/kibana/src/core/server/plugins/plugin.ts:108:26)
    at PluginsSystem.setupPlugins (/Users/yakhinvadim/Projects/kibana/src/core/server/plugins/plugins_system.ts:129:40)
    at PluginsService.setup (/Users/yakhinvadim/Projects/kibana/src/core/server/plugins/plugins_service.ts:171:52)
    at Server.setup (/Users/yakhinvadim/Projects/kibana/src/core/server/server.ts:282:26)
    at Root.setup (/Users/yakhinvadim/Projects/kibana/src/core/server/root/index.ts:52:14)
    at bootstrap (/Users/yakhinvadim/Projects/kibana/src/core/server/bootstrap.ts:101:5)
    at Command.<anonymous> (/Users/yakhinvadim/Projects/kibana/src/cli/serve/serve.js:244:5)

Terminating process...
 server crashed  with status code 1

Let me know if I can help with the debugging. Happy to pair.

EDIT: Just found that a PR has already been created to fix it! #120659

kibanamachine · 2021-12-08T21:05:00Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 120110 or prevent reminders by adding the backport:skip label.

tsullivan · 2021-12-08T21:54:52Z

Removed the 8.0 version label as we are past feature freeze for that branch

…c#120110) * Add screenshotting plugin * Move screenshotting plugin configuration options * Remove unused browser type configuration option # Conflicts: # packages/kbn-optimizer/limits.yml # x-pack/plugins/reporting/server/export_types/common/generate_png.ts # x-pack/plugins/reporting/server/export_types/png/execute_job/index.ts # x-pack/plugins/reporting/server/export_types/png_v2/execute_job.ts # x-pack/plugins/reporting/server/export_types/printable_pdf/lib/tracker.ts # x-pack/plugins/reporting/server/export_types/printable_pdf_v2/lib/tracker.ts # x-pack/plugins/reporting/server/lib/screenshots/observable.ts # x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts

… (#120937) * Add screenshotting plugin * Move screenshotting plugin configuration options * Remove unused browser type configuration option # Conflicts: # packages/kbn-optimizer/limits.yml # x-pack/plugins/reporting/server/export_types/common/generate_png.ts # x-pack/plugins/reporting/server/export_types/png/execute_job/index.ts # x-pack/plugins/reporting/server/export_types/png_v2/execute_job.ts # x-pack/plugins/reporting/server/export_types/printable_pdf/lib/tracker.ts # x-pack/plugins/reporting/server/export_types/printable_pdf_v2/lib/tracker.ts # x-pack/plugins/reporting/server/lib/screenshots/observable.ts # x-pack/plugins/reporting/server/lib/screenshots/observable_handler.ts

…c#120110) * Add screenshotting plugin * Move screenshotting plugin configuration options * Remove unused browser type configuration option

dokmic added release_note:enhancement review (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead v8.0.0 v8.1.0 Team:AppServicesUx labels Dec 1, 2021

dokmic force-pushed the feature/KS-626 branch 2 times, most recently from 00292ee to 3f508e1 Compare December 1, 2021 17:45

tsullivan reviewed Dec 1, 2021

View reviewed changes

x-pack/plugins/reporting/server/core.ts Outdated Show resolved Hide resolved

tsullivan reviewed Dec 1, 2021

View reviewed changes

x-pack/plugins/screenshotting/server/browsers/chromium/paths.ts Outdated Show resolved Hide resolved

dokmic force-pushed the feature/KS-626 branch 2 times, most recently from 823b8b9 to 66391b4 Compare December 3, 2021 13:59

jloleysens approved these changes Dec 3, 2021

View reviewed changes

dokmic force-pushed the feature/KS-626 branch 4 times, most recently from 6eb0310 to 6032c78 Compare December 3, 2021 17:48

tsullivan reviewed Dec 3, 2021

View reviewed changes

tsullivan approved these changes Dec 3, 2021

View reviewed changes

dokmic force-pushed the feature/KS-626 branch from 6032c78 to 7b72877 Compare December 3, 2021 22:48

dokmic marked this pull request as ready for review December 6, 2021 08:59

dokmic requested review from a team as code owners December 6, 2021 08:59

Add screenshotting plugin

c844162

jloleysens self-requested a review December 6, 2021 11:54

jloleysens approved these changes Dec 6, 2021

View reviewed changes

TinaHeiligers approved these changes Dec 6, 2021

View reviewed changes

jbudz approved these changes Dec 6, 2021

View reviewed changes

spalger reviewed Dec 6, 2021

View reviewed changes

dmlemeshko approved these changes Dec 6, 2021

View reviewed changes

dokmic merged commit 903e75e into elastic:main Dec 6, 2021

tsullivan mentioned this pull request Dec 7, 2021

[Reporting] fix unsupported platform crash #120659

Merged

1 task

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 8, 2021

tsullivan removed the v8.0.0 label Dec 8, 2021

matschaffer mentioned this pull request Dec 9, 2021

A silly change for trying out cloud builds #120709

Closed

9 tasks

mshustov mentioned this pull request Dec 9, 2021

rename deprecation factory does not remove an empty object #120876

Closed

dokmic added the v8.0.0 label Dec 9, 2021

dokmic mentioned this pull request Dec 9, 2021

[8.0] [Reporting] Decouple screenshotting plugin from the reporting (#120110) #120937

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 9, 2021

afharo mentioned this pull request Dec 14, 2021

[PluginService] Register all deprecations before running isEnabledPath #121084

Merged

2 tasks

tsullivan mentioned this pull request Dec 27, 2021

[Screenshotting] Have Darwin download Chromium at startup, add ARM support for Darwin #122057

Merged

4 tasks

tsullivan mentioned this pull request Mar 10, 2022

[Reporting] Update docs to use screenshotting config properties #127518

Merged

dokmic deleted the feature/KS-626 branch July 29, 2022 09:40

tsullivan mentioned this pull request Jul 3, 2024

[Reporting/Screenshotting] Backport screenshotting plugin to 7.17 #187512

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reporting] Decouple screenshotting plugin from the reporting #120110

[Reporting] Decouple screenshotting plugin from the reporting #120110

dokmic commented Dec 1, 2021 •

edited

Loading

tsullivan Dec 1, 2021 •

edited

Loading

jloleysens left a comment •

edited

Loading

jloleysens Dec 2, 2021

tsullivan Dec 3, 2021

tsullivan left a comment

elasticmachine commented Dec 6, 2021

dokmic commented Dec 6, 2021

jloleysens left a comment

kibana-ci commented Dec 6, 2021

API count

References to deprecated APIs

TinaHeiligers left a comment

spalger Dec 6, 2021

spalger Dec 6, 2021

dokmic Dec 6, 2021

dmlemeshko left a comment

yakhinvadim commented Dec 7, 2021 •

edited

Loading

kibanamachine commented Dec 8, 2021

tsullivan commented Dec 8, 2021

[Reporting] Decouple screenshotting plugin from the reporting #120110

[Reporting] Decouple screenshotting plugin from the reporting #120110

Conversation

dokmic commented Dec 1, 2021 • edited Loading

Summary

Checklist

For maintainers

tsullivan Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

jloleysens left a comment • edited Loading

Choose a reason for hiding this comment

jloleysens Dec 2, 2021

Choose a reason for hiding this comment

tsullivan Dec 3, 2021

Choose a reason for hiding this comment

tsullivan left a comment

Choose a reason for hiding this comment

elasticmachine commented Dec 6, 2021

dokmic commented Dec 6, 2021

jloleysens left a comment

Choose a reason for hiding this comment

kibana-ci commented Dec 6, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

References to deprecated APIs

History

TinaHeiligers left a comment

Choose a reason for hiding this comment

spalger Dec 6, 2021

Choose a reason for hiding this comment

spalger Dec 6, 2021

Choose a reason for hiding this comment

dokmic Dec 6, 2021

Choose a reason for hiding this comment

dmlemeshko left a comment

Choose a reason for hiding this comment

yakhinvadim commented Dec 7, 2021 • edited Loading

kibanamachine commented Dec 8, 2021

tsullivan commented Dec 8, 2021

dokmic commented Dec 1, 2021 •

edited

Loading

tsullivan Dec 1, 2021 •

edited

Loading

jloleysens left a comment •

edited

Loading

yakhinvadim commented Dec 7, 2021 •

edited

Loading